Skip to content

GCI1066 [Team TREE][2025] - For logging format, prefer using %s with kwargs instead of builtin formatter \"\".format() or f\"\"" #84

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

fabienhusseperso
Copy link

WIP

@@ -0,0 +1,34 @@
package org.greencodeinitiative.creedengo.python.checks;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add corresponding header :
/*

  • creedengo - Python language - Provides rules to reduce the environmental footprint of your Python programs
  • Copyright © 2024 Green Code Initiative (https://green-code-initiative.org)
  • This program is free software: you can redistribute it and/or modify
  • it under the terms of the GNU General Public License as published by
  • the Free Software Foundation, either version 3 of the License, or
  • (at your option) any later version.
  • This program is distributed in the hope that it will be useful,
  • but WITHOUT ANY WARRANTY; without even the implied warranty of
  • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
  • GNU General Public License for more details.
  • You should have received a copy of the GNU General Public License
  • along with this program. If not, see http://www.gnu.org/licenses/.
    */

@@ -0,0 +1,14 @@
package org.greencodeinitiative.creedengo.python.checks;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add corresponding header :
/*

  • creedengo - Python language - Provides rules to reduce the environmental footprint of your Python programs
  • Copyright © 2024 Green Code Initiative (https://green-code-initiative.org)
  • This program is free software: you can redistribute it and/or modify
  • it under the terms of the GNU General Public License as published by
  • the Free Software Foundation, either version 3 of the License, or
  • (at your option) any later version.
  • This program is distributed in the hope that it will be useful,
  • but WITHOUT ANY WARRANTY; without even the implied warranty of
  • MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
  • GNU General Public License for more details.
  • You should have received a copy of the GNU General Public License
  • along with this program. If not, see http://www.gnu.org/licenses/.
    */


protected static final String MESSAGE_RULE = "For logging format, prefer using %s with kwargs instead of builtin formatter \"\".format() or f\"\"";
private static final Pattern PATTERN_FORMAT = Pattern.compile("f['\"].*['\"]");
private static final Pattern PATTERN_LOGGER = Pattern.compile("logging|logger");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest making a version that is insensitive to the variable name because the user can define it, so decteion won't work:

here's what I suggest:
public class DetectBadLoggingFormatInterpolation extends PythonSubscriptionCheck {

protected static final String MESSAGE_RULE = "For logging format, prefer using %s with kwargs instead of builtin formatter \"\".format() or f\"\"";
private static final Pattern PATTERN_FORMAT = Pattern.compile("f['\"].*['\"]");
private static final Pattern PATTERN_LOGGING_METHOD = Pattern.compile("info|debug|error");
private static final String LOGGER_FUNCCTION = "logging.getLogger";
private final Set<String> loggerNames = new HashSet<>();

@Override
public void initialize(Context context) {
    context.registerSyntaxNodeConsumer(Tree.Kind.CALL_EXPR, this::visitNodeString);
    context.registerSyntaxNodeConsumer(ASSIGNMENT_STMT, this::handleLoggerAssignment);
}

public void visitNodeString(SubscriptionContext ctx) {
    CallExpression callExpression = (CallExpression) ctx.syntaxNode();
    Expression callee = callExpression.callee();

    if (!(callee instanceof QualifiedExpression)) {
        return;
    }

    QualifiedExpression qualifiedExpression = (QualifiedExpression) callee;
    Expression qualifier = qualifiedExpression.qualifier();

    if (!(qualifier instanceof Name)) {
        return;
    }

    Name qualifierName = (Name) qualifier;

    if (PATTERN_LOGGING_METHOD.matcher(qualifiedExpression.name().name()).matches()
            && loggerNames.contains(qualifierName.name())
            && !callExpression.arguments().isEmpty()
            && callExpression.arguments().get(0) instanceof RegularArgument) {

        Expression argExpr = ((RegularArgument) callExpression.arguments().get(0)).expression();

        if (argExpr.firstToken() != null && PATTERN_FORMAT.matcher(argExpr.firstToken().value()).matches()) {
            ctx.addIssue(callExpression, MESSAGE_RULE);
        }
    }
}

public void handleLoggerAssignment(SubscriptionContext ctx) {
    var assignmentStmt = (AssignmentStatement) ctx.syntaxNode();
    var value = assignmentStmt.assignedValue();

    if (value.is(CALL_EXPR) && isLoggerCreation((CallExpression) value)) {
        String variableName = Utils.getVariableName(ctx);
        if (variableName != null) {
            loggerNames.add(variableName);
            System.out.println("Logger created with name: " + variableName);
        }
    }
}

private boolean isLoggerCreation(CallExpression callExpression) {
    return LOGGER_FUNCCTION.equals(Utils.getQualifiedName(callExpression));
}

}


#logging.info("Hello {}".format(name)) # Noncompliant {{For logging format, prefer using %s with kwargs instead of builtin formatter "".format() or f""}}
logger.debug(f'Hello {name}') # Noncompliant {{For logging format, prefer using %s with kwargs instead of builtin formatter "".format() or f""}}
logger.error(f"Hello {name}") # Noncompliant {{For logging format, prefer using %s with kwargs instead of builtin formatter "".format() or f""}}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you modify the variable name so that it is not modifiable, you will have to add the corresponding tests, for example :

my_logger = logging.getLogger()
user = "admin"
ip = "192.168.0.1"

my_logger.debug(f"User {user} attempted login") # Noncompliant {{For logging format, prefer using %s with kwargs instead of builtin formatter "".format() or f""}}
my_logger.error(f"Blocked IP: {ip}") # Noncompliant {{For logging format, prefer using %s with kwargs instead of builtin formatter "".format() or f""}}

my_logger.info("Static log with no variables")

my_logger.info("User %s connected", user)
my_logger.warning("IP %s is blocked", ip)
my_logger.debug("User %s attempted login", user)
my_logger.error("Blocked IP: %s", ip)
my_logger.critical("CRITICAL issue with user %s on IP %s", user, ip)

if True:
my_logger.debug(f"Conditional debug for {user}") # Noncompliant {{For logging format, prefer using %s with kwargs instead of builtin formatter "".format() or f""}}

with open("log.txt", "w") as f:
my_logger.info("Opened file for logging")

class MyService:
def init(self):
self.logger = logging.getLogger("service")

def report(self, user):
    self.logger.info("Reporting user %s", user)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants